Fix missing script time for some functions in profiler #75623
Conversation
reach-satori
force-pushed
the
profiler_for_builtins
branch
4 times, most recently
from
b990e5c
to
eb97445
Compare
reach-satori
force-pushed
the
profiler_for_builtins
branch
2 times, most recently
from
ff67d1c
to
5ad630f
Compare
|
Overall looks okay but I do have some nitpicks. The major one is calling this "builtins" which I think isn't clear. In general they are called "native". I would prefer to use |
modules/gdscript/gdscript_vm.cpp
Outdated
| // In some cases, non-verified builtin functions end up in OPCODE_CALL. | ||
| // When that happens, base_class is empty, so we can use this to stop | ||
| // the time from being subtracted in that case | ||
| // |
There was a problem hiding this comment.
I don't see why this would be the case. They should still have a base Object which will always have a class name (unless they are freed or it's null). The only exception are calls on built-in types (Vector2, String, etc.) which don't have a base Object.
There was a problem hiding this comment.
You're right, that was my mistake. I can't figure out a nice way to distinguish native from user GDScript code at this point in the VM without bigger changes. After thinking about it for a while and testing some things, the best I could find was:
if (!base_obj || !base_obj->get_script_instance() || !base_obj->get_script_instance()->has_method(*methodname))But I don't know, there seem to be a ton of potential corner cases. Can you think of a scenario where this would fail? Or a decent alternative? I'll spend some more time testing this more thoroughly regardless.
There was a problem hiding this comment.
I don't think there are any good solutions. At this point the VM does not really know who owns the function being called (whether a script or a native class). It only delegates to Variant which does the actual dynamic dispatch.
I now wonder if this profiling should be done by Variant instead of the GDScript VM.
reach-satori
force-pushed
the
profiler_for_builtins
branch
3 times, most recently
from
7fbbb80
to
1409fc4
Compare
reach-satori
force-pushed
the
profiler_for_builtins
branch
2 times, most recently
from
a1349bb
to
d405cea
Compare
|
As per @vnen's comment, there seems to be no clean way to do this properly without bigger changes that would probably require a proposal first (and/or would be over my head by quite a bit atm), but I think I managed to get some of the way there using ClassDB. As it is, according to my testing, this works as intended with straightforward native calls from GDScript, but fails in a few important cases. Those cases are: call_deferred(), call(), and new(), because I couldn't find a way to find out if what they're calling ends up in native code or GDScript code. So I've left them unchanged and they'll miss time, same as the current profiler does for all native functions. In case someone eventually wants to look into it, here's also the small testing project I was using. It's essentially just a loop that calls one from a selection of function configurations, with a menu, so you can look at the profiler behaviour. Although, if it seems like nothing useful can come of this, feel free to close this PR. |
reach-satori
force-pushed
the
profiler_for_builtins
branch
from
d405cea
to
28d3ba3
Compare
|
Please don't merge See this documentation, if you need help with rebasing. |
reach-satori
force-pushed
the
profiler_for_builtins
branch
from
48e3cee
to
b168e37
Compare
|
Hi @reach-satori! Thanks for your first PR. Don't hesitate to join our developer chat! We could help you with git and your PR. |
reach-satori
force-pushed
the
profiler_for_builtins
branch
from
b168e37
to
fa70a40
Compare
|
@adamscott |
|
Happy to see that you resolved your issue! |
reach-satori
force-pushed
the
profiler_for_builtins
branch
from
fa70a40
to
1c76844
Compare
reach-satori
force-pushed
the
profiler_for_builtins
branch
from
1c76844
to
0658b1a
Compare
Fixes the issue by adding a mechanism by which the functions that were previously disappearing can be profiled too. This is optional with an editor setting, since collecting more information naturally slows the engine further while profiling. Fixes godotengine#23715, godotengine#40251, godotengine#29049
YuriSizov
force-pushed
the
profiler_for_builtins
branch
from
f55a271
to
f1cc14d
Compare
|
Applied suggestions and rebased on your behalf. |
|
Thanks, and congrats on your first merged Godot contribution! |
This PR adds the (toggleable in settings) ability to profile non-gdscript functions , and makes it so the time spent in them no longer disappears
Since it's a pretty big pr and touches a lot of code, I'll briefly explain what the problem was and how I solved it:
Essentially, the script profile information is saved only in GDScriptFunction. But when something that's not a GDScriptFunction is called from there, the code assumed it would profile normally anyway and subtracted the time spent in it to get a self-time, and this wrong information caused the bug.
I've added an additional hashmap attribute to GDScriptFunction::Profile that instead saves the information on any function directly under it in the stack that has no capability to profile itself. In profiling_get_frame_data these sub-profiles then get converted to normal profile frames and sent off as usual.
The changes in ScriptLanguage and GDScriptLanguage are just to get the information on whether profiling is enabled or not from the settings to the VM, following the same mold of the profiler enabling/disabling. I thought this was necessary because collecting more information naturally slows down the execution during profiling.
Fixes #23715, fixes #40251, fixes #29049